Skip to content

Add validation to MultiRecord.wrsamp for segment cohesion - #559

Open
briangow wants to merge 4 commits into
mainfrom
bg_enable_seg_cohesion
Open

briangow wants to merge 4 commits into
mainfrom
bg_enable_seg_cohesion

Conversation

@briangow

Copy link
Copy Markdown
Contributor

As mentioned in #557, we should add segment validation when writing multi-segment records. This PR adds pre-write validation to MultiRecord.wrsamp() by activating the previously unused _check_segment_cohesion() method, which is now called before any files are written.

The validation includes checks for basic segment cohesion (matching n_seg, sampling frequencies, and segment lengths) as well as new validation for expanded signals. For segments with e_d_signal or e_p_signal, the method now verifies that samps_per_frame is properly set for all channels and that expanded signal lengths match expected values (samps_per_frame[ch] * sig_len). This provides clear, descriptive error messages when segments are misconfigured, preventing invalid multi-segment records from being written.

@briangow
briangow force-pushed the bg_enable_seg_cohesion branch from 1257006 to 51faaf0 Compare October 29, 2026 20:21
@briangow
briangow requested a review from tompollard October 29, 2026 20:24

@tompollard tompollard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Brian, checking all the segments before writing anything is a good change. On main, a segment whose length doesn't match seg_len gets written without any error, and a bad segment partway through leaves half-written files. This PR fixes both.

I tried a few cases locally, and two writes that work on main are now rejected:

  1. The expanded check runs even when expanded=False. A segment with a valid d_signal and an out-of-date e_d_signal now fails, even though only d_signal is written. The check also falls back to e_p_signal, which is never written. Could we pass expanded into _check_segment_cohesion and only check e_d_signal when it's True?
  2. samps_per_frame=[None] is rejected. Record.wrsamp(expanded=True) treats a missing value as 1 (check_sig_cohesion in _signal.py), so a segment that writes fine on its own now fails inside a multi-segment record. It would be good to treat it the same way here.

Both of these come from having the expanded-length rule in two places. Another option is to drop the new block and run each segment's existing checks (its header field checks plus check_sig_cohesion) in a first pass, then write everything in a second pass. That keeps the "check before writing" behaviour with a single rule.

A few smaller things:

  • Gap segments (None, e.g. from reading a variable-layout record with sampto) now fail with 'NoneType' object has no attribute 'fs'. That isn't worse than before, but a clear error saying gap segments can't be written yet would help.
  • There are no tests for MultiRecord.wrsamp yet. It would be good to add:
    • a round trip of 041s
    • a seg_len mismatch
    • a bad segment after a good one, checking that no files are written
    • the two cases above that should be accepted
  • Could you add a changelog entry to docs/changes.rst?
  • A few of the new error message lines are over 88 characters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants